-
-
Notifications
You must be signed in to change notification settings - Fork 1
Added utility for loading tiered config from file system. #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful, there are several mistakes here that should not be.
We will also want to get the CLI with this.
We can break this up into two PRs:
- This one.
- One that adds the CLI.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are generally looking good.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good.
We also need some text in the README covering how config works.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Looking pretty good.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is looking much better.
I did not fully comment the new CLI option table,
since it needs some big changes.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is getting pretty close.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting better.
The source intros really help.
eriq-augustine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Implemented a function that hierarchically loads configuration keys and values from JSON config files and CLI arguments, with an option to specify the target file name.